Implement request timeout middleware #46115

Kahbazi · 2023-01-15T22:16:20Z

Implement request timeout middleware

Implement a middleware which links a cancellation token to HttpContext.RequestAborted in order to handle request timeout.

Description

The middleware looks up the endpoint metadata and get the timespan from it. The Order is RequestTimeoutPolicy, RequestTimeoutAttribute and if none of them are available, it will check the RequestTimeoutOptions.DefaultTimeout. If the DisableRequestTimeoutAttribute metadata is available then the middleware would be skipped.

Questions:

I didn't quite understand the need of RequestTimeoutPolicy as a metadata. Why not just add a constructor that takes Timespan to RequestTimeoutAttribute and avoid looking up for both metadata?
Should the CancellationTokenSources be reused? Something like ActivityCancellationTokenSource.
How should I test this? My understanding is that we should not use Task.Delay in tests, but I see no other way to test this. Should I just use that?

Fixes #45732

ghost · 2023-01-15T22:16:28Z

Thanks for your PR, @Kahbazi. Someone from the team will get assigned to your PR shortly and we'll get it reviewed.

src/Http/Http/src/Timeouts/RequestTimeoutsMiddleware.cs

src/Http/Http/src/Timeouts/RequestTimeoutsIEndpointConventionBuilderExtensions.cs

src/Http/Http/src/Timeouts/RequestTimeoutsMiddleware.cs

JamesNK

This is basically like a gRPC deadline on the server. You can see the main class for implementing that here: https://github.com/grpc/grpc-dotnet/blob/948be08b088fbf449982b731c8e1a7cf8abb1965/src/Grpc.AspNetCore.Server/Internal/ServerCallDeadlineManager.cs

gRPC implementation is a lot more complicated than needed because I wanted to avoid allocating a CancellationTokenSource unless requested. Also, it supports timeouts greater than the duration of CancellationTokenSource.

Edit: I read the original issue and see a significant difference between this and gRPC deadlines. Unlike a deadline, this is cooperative and doesn't cancel the original request.

src/Http/Http/src/Timeouts/RequestTimeoutsMiddleware.cs

JamesNK · 2023-01-16T13:37:41Z

src/Http/Http/src/Timeouts/RequestTimeoutsMiddleware.cs

+        {
+            var originalToken = context.RequestAborted;
+            var timeoutCts = new CancellationTokenSource(timespan.Value);
+            var linkedCts = CancellationTokenSource.CreateLinkedTokenSource(context.RequestAborted, timeoutCts.Token);


Creating a linked token source is quite expensive. There are now 3 token sources for the request.

The original RequestAborted CTS

The timeout CTS

The linked CTS

You can get that down to two CTS. Replace the linked CTS with subscribing to the original CTS's token, and call timeoutCts.Cancel() in the callback. Gotchas are you have to add thread safety between timeoutCts.Cancel() and timeoutCts.Dispose() (canceling a disposed CTS errors), and unregister the subscription.

I think we need to get this down to 0 (amortized). We need to use an ObjectPool<CancellationTokenSource> or (https://github.com/dotnet/aspnetcore/blob/4535ea1263e9a24ca8d37b7266797fe1563b8b12/src/Shared/CancellationTokenSourcePool.cs). This combined with RegisterForDispose should get the allocations per request down (removes the async state machine).

I'm tempted to replace this custom pool with the ObjectPool since we just improved the performance.

cc @BrennanConroy

We could use IPersistentState to store the CTS on the HttpContext. It will be cached along with the context.

That avoids any locking and contention from a shared pool.

Nice. We can try that.

@JamesNK I'm still trying to figure out your solution. Are you suggesting to store the Original CTS from here in IPersistentState

aspnetcore/src/Servers/Kestrel/Core/src/Internal/Http/HttpProtocol.cs

Line 294 in 0e51178

_abortedCts = new CancellationTokenSource();

and then call CancelAfter(timespan) ?

@Tratcher Any guidance on this thread? I would like to do this one in this PR if I can.

get the benchmarks working first, then we can measure the impact of this change

Sure. Could you please point me to a benchmark sample and what do I need to benchmark exactly? I'm thinking a pipeline with and without the request timeouts middleware?

https://github.com/dotnet/aspnetcore/blob/main/src/Middleware/RequestDecompression/perf/Microbenchmarks/RequestDecompressionMiddlewareBenchmark.cs

An endpoint with no metadata, no default timeout

a default timeout

default overridden by disable attribute

a timeout set on an endpoint

a named policy

a very short timeout that fires

Tratcher · 2023-01-17T21:54:09Z

I didn't quite understand the need of RequestTimeoutPolicy as a metadata. Why not just add a constructor that takes Timespan to RequestTimeoutAttribute and avoid looking up for both metadata?

We didn't want a Timespan constructor for RequestTimeoutAttribute because Timespan's can't be used as attribute input.

Tratcher · 2023-01-17T21:57:24Z

Should the CancellationTokenSources be reused? Something like ActivityCancellationTokenSource.

How should I test this? My understanding is that we should not use Task.Delay in tests, but I see no other way to test this. Should I just use that?

These two questions go together. Factor out the token creation to an internal service. That service would be responsible for both recycling tokens, as well as mocking them when needed. The mock implementation can use a manually advancing clock rather than a timer.

src/Http/Http/src/Timeouts/DisableRequestTimeoutAttribute.cs

src/Http/Http/src/Timeouts/RequestTimeoutsMiddleware.cs

src/Http/Http/src/Timeouts/RequestTimeoutAttribute.cs

src/Http/Http/src/Timeouts/RequestTimeoutsMiddleware.cs

Tratcher

Revert src/submodules/googletest

src/Http/Http/src/Microsoft.AspNetCore.Http.csproj

src/Http/Http/src/Timeouts/RequestTimeoutsMiddleware.cs

davidfowl · 2023-01-20T04:59:50Z

We'll want a performance test for this middleware as well. We need to make it as pay for play as possible. Most requests won't be timing out, so we should optimize for that.

We also need to potentially do something with long running requests cc @BrennanConroy

JamesNK · 2023-01-20T05:28:03Z

How long is a long-running request? Hours or days?

What I did for gRPC + long timeouts was to have a Timer that reschedules itself until the timeout is zero. However, there is an extra allocation there.

What you can do is have different paths for short timeouts (milliseconds less than 2^32) and long timeouts.

Short = optimized = just use a resettable CancellationTokenSource, and cache it
Long = Timer allocation = rescheduling timer route.

If a request is long-running then per-request allocations aren't a big concern.

src/Http/Http/src/Timeouts/RequestTimeoutsMiddleware.cs

davidfowl · 2023-01-20T16:41:18Z

How long is a long-running request? Hours or days?

Something this middleware should ignore. I'm thinking about streaming requests:

WebSockets
GRPC streams
SSE
LongPolling
SignalR
WebTransport

The ability to set a global default timeout devoid of a specific endpoint breaks those scenarios. Should there be a feature to disable the timeout (similar to what we have for response buffering).

src/Http/Http/src/Timeouts/CancellationTokenLinker.cs

src/Http/Http/src/PublicAPI.Unshipped.txt

src/Http/Http/src/Timeouts/RequestTimeoutsMiddleware.cs

Tratcher · 2023-01-20T20:00:34Z

The ability to set a global default timeout devoid of a specific endpoint breaks those scenarios. Should there be a feature to disable the timeout (similar to what we have for response buffering).

Timeouts can already be disabled per endpoint via metadata. I understand that can be tedious/redundant, but I hesitate to try to special case all of those scenarios in the middleware. Having an opt-out feature is a bit odd because the timeout already started and may fire before it's disabled.

Proposal:

For WebTransport and WebSockets/Upgrade we do have the ability to stop the timer when they are accepted by shimming those features.
SignalR can configure/disable timeouts for its own endpoints as needed.
Long Polling already has a timeout mechanism, it can opt out of this one via metadata.
SSE would likely opt out as well.
GRPC is an interesting case because there are both streamed and non-streamed requests and middleware can't tell the difference. @JamesNK when gRPC is generating endpoints does it know which endpoints are for streaming so it can opt out of timeouts for those endpoints?

src/Http/Http/src/Timeouts/RequestTimeoutsMiddleware.cs

src/Http/Http/src/Timeouts/CancellationTokenLinker.cs

src/Http/Http/src/Timeouts/IHttpRequestTimeoutFeature.cs

src/Http/Http/src/Timeouts/LoggerExtensions.cs

src/Http/Http/src/Timeouts/RequestTimeoutOptions.cs

adityamandaleeka · 2023-03-16T02:15:00Z

@Kahbazi I'm pushing a few cleanup updates to this PR so we can get this merged quickly.

src/Servers/Kestrel/tools/CodeGenerator/HttpProtocolFeatureCollection.cs

adityamandaleeka · 2023-03-16T20:10:10Z

Looks like this is merged. Thank you for all your work on this @Kahbazi!

Kahbazi · 2023-03-16T20:11:34Z

Looks like this is merged. Thank you for all your work on this @Kahbazi!

You're welcome. Sorry I couldn't complete this on time. We are approaching new year holidays in here.

ghost · 2023-03-16T20:11:38Z

Hi @Kahbazi. It looks like you just commented on a closed PR. The team will most probably miss it. If you'd like to bring something important up to their attention, consider filing a new issue and add enough details to build context.

adityamandaleeka · 2023-03-16T20:15:34Z

Looks like this is merged. Thank you for all your work on this @Kahbazi!

You're welcome. Sorry I couldn't complete this on time. We are approaching new year holidays in here.

No worries at all. And happy new year!

src/Http/Http/src/Timeouts/RequestTimeoutPolicy.cs

ghost added area-runtime community-contribution Indicates that the PR has been added by a community member labels Jan 15, 2023

davidfowl reviewed Jan 15, 2023

View reviewed changes

src/Http/Http/src/Timeouts/RequestTimeoutsMiddleware.cs Outdated Show resolved Hide resolved

davidfowl reviewed Jan 16, 2023

View reviewed changes

src/Http/Http/src/Timeouts/RequestTimeoutsIEndpointConventionBuilderExtensions.cs Outdated Show resolved Hide resolved

davidfowl reviewed Jan 16, 2023

View reviewed changes

src/Http/Http/src/Timeouts/RequestTimeoutsIEndpointConventionBuilderExtensions.cs Outdated Show resolved Hide resolved

davidfowl reviewed Jan 16, 2023

View reviewed changes

src/Http/Http/src/Timeouts/RequestTimeoutsIEndpointConventionBuilderExtensions.cs Outdated Show resolved Hide resolved

davidfowl reviewed Jan 16, 2023

View reviewed changes

src/Http/Http/src/Timeouts/RequestTimeoutsMiddleware.cs Outdated Show resolved Hide resolved

JamesNK reviewed Jan 16, 2023

View reviewed changes

src/Http/Http/src/Timeouts/RequestTimeoutsMiddleware.cs Outdated Show resolved Hide resolved

JamesNK reviewed Jan 16, 2023

View reviewed changes

Tratcher self-assigned this Jan 17, 2023

Tratcher reviewed Jan 17, 2023

View reviewed changes

src/Http/Http/src/Timeouts/DisableRequestTimeoutAttribute.cs Outdated Show resolved Hide resolved

src/Http/Http/src/Timeouts/RequestTimeoutsMiddleware.cs Outdated Show resolved Hide resolved

src/Http/Http/src/Timeouts/RequestTimeoutsMiddleware.cs Outdated Show resolved Hide resolved

campersau reviewed Jan 18, 2023

View reviewed changes

src/Http/Http/src/Timeouts/RequestTimeoutsMiddleware.cs Outdated Show resolved Hide resolved

src/Http/Http/src/Timeouts/RequestTimeoutAttribute.cs Outdated Show resolved Hide resolved

Tratcher reviewed Jan 18, 2023

View reviewed changes

src/Http/Http/src/Timeouts/RequestTimeoutsMiddleware.cs Outdated Show resolved Hide resolved

src/Http/Http/src/Timeouts/RequestTimeoutsMiddleware.cs Outdated Show resolved Hide resolved

src/Http/Http/src/Timeouts/RequestTimeoutsMiddleware.cs Outdated Show resolved Hide resolved

tekian reviewed Jan 19, 2023

View reviewed changes

src/Http/Http/src/Timeouts/RequestTimeoutsMiddleware.cs Show resolved Hide resolved

Kahbazi force-pushed the kahbazi/timeoutMiddleware branch from 81d90c5 to dc29a51 Compare January 19, 2023 15:11

Tratcher reviewed Jan 19, 2023

View reviewed changes

src/Http/Http/src/Microsoft.AspNetCore.Http.csproj Outdated Show resolved Hide resolved

src/Http/Http/src/Timeouts/RequestTimeoutsMiddleware.cs Show resolved Hide resolved

davidfowl reviewed Jan 20, 2023

View reviewed changes

src/Http/Http/src/Timeouts/RequestTimeoutsMiddleware.cs Outdated Show resolved Hide resolved

BrennanConroy reviewed Jan 20, 2023

View reviewed changes

src/Http/Http/src/Timeouts/CancellationTokenLinker.cs Outdated Show resolved Hide resolved

src/Http/Http/src/PublicAPI.Unshipped.txt Outdated Show resolved Hide resolved

src/Http/Http/src/Timeouts/RequestTimeoutsMiddleware.cs Outdated Show resolved Hide resolved

Tratcher reviewed Jan 20, 2023

View reviewed changes

src/Http/Http/src/Timeouts/RequestTimeoutsMiddleware.cs Show resolved Hide resolved

Tratcher reviewed Jan 20, 2023

View reviewed changes

src/Http/Http/src/Timeouts/RequestTimeoutsMiddleware.cs Outdated Show resolved Hide resolved

Kahbazi commented Jan 23, 2023

View reviewed changes

src/Http/Http/src/Timeouts/RequestTimeoutsMiddleware.cs Outdated Show resolved Hide resolved

Kahbazi force-pushed the kahbazi/timeoutMiddleware branch from 36ae661 to 3d11e63 Compare January 23, 2023 20:15

BrennanConroy reviewed Jan 23, 2023

View reviewed changes

src/Http/Http/src/Timeouts/CancellationTokenLinker.cs Outdated Show resolved Hide resolved

Kahbazi force-pushed the kahbazi/timeoutMiddleware branch from 96ac92f to 17dffdd Compare March 9, 2023 21:44

Kahbazi requested a review from captainsafia as a code owner March 9, 2023 21:44

BrennanConroy reviewed Mar 10, 2023

View reviewed changes

src/Http/Http/src/Timeouts/IHttpRequestTimeoutFeature.cs Outdated Show resolved Hide resolved

src/Http/Http/src/Timeouts/LoggerExtensions.cs Show resolved Hide resolved

BrennanConroy reviewed Mar 10, 2023

View reviewed changes

src/Http/Http/src/Timeouts/RequestTimeoutOptions.cs Outdated Show resolved Hide resolved

adityamandaleeka added 3 commits March 15, 2023 19:08

Fix typos/casing.

4250e31

Doc comments.

0e71e8e

Rename test file.

e7839c6

adityamandaleeka added 2 commits March 15, 2023 19:18

Misc cleanup.

61eb2ee

Add to HttpProtocolFeatureCollection.

4a29159

BrennanConroy reviewed Mar 16, 2023

View reviewed changes

src/Servers/Kestrel/tools/CodeGenerator/HttpProtocolFeatureCollection.cs Show resolved Hide resolved

adityamandaleeka and others added 3 commits March 15, 2023 20:06

More edits.

2515bf1

Re-run generator for feature collection.

2e5a6a1

Fix namespaces

8f28de5

Tratcher enabled auto-merge (squash) March 16, 2023 16:19

adityamandaleeka disabled auto-merge March 16, 2023 16:46

Tratcher enabled auto-merge (squash) March 16, 2023 16:53

Fix test

fb7422b

Tratcher merged commit f9cc9e1 into dotnet:main Mar 16, 2023

ghost added this to the 8.0-preview3 milestone Mar 16, 2023

Kahbazi deleted the kahbazi/timeoutMiddleware branch March 18, 2023 08:31

adityamandaleeka mentioned this pull request Apr 10, 2023

Add docs for request timeouts middleware dotnet/AspNetCore.Docs#28934

Closed

MoienTajik reviewed Apr 19, 2023

View reviewed changes

src/Http/Http/src/Timeouts/RequestTimeoutPolicy.cs Show resolved Hide resolved

amcasey added area-networking Includes servers, yarp, json patch, bedrock, websockets, http client factory, and http abstractions and removed area-runtime labels Jun 6, 2023

github-actions bot locked and limited conversation to collaborators Dec 8, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement request timeout middleware #46115

Implement request timeout middleware #46115

Kahbazi commented Jan 15, 2023

ghost commented Jan 15, 2023

JamesNK left a comment •

edited

Loading

JamesNK Jan 16, 2023 •

edited

Loading

davidfowl Jan 16, 2023

davidfowl Jan 16, 2023

JamesNK Jan 17, 2023

davidfowl Jan 17, 2023

Kahbazi Jan 23, 2023

Kahbazi Jan 25, 2023

Tratcher Jan 25, 2023

Kahbazi Jan 25, 2023

Tratcher Jan 25, 2023

Tratcher commented Jan 17, 2023

Tratcher commented Jan 17, 2023 •

edited

Loading

Tratcher left a comment

davidfowl commented Jan 20, 2023

JamesNK commented Jan 20, 2023 •

edited

Loading

davidfowl commented Jan 20, 2023

Tratcher commented Jan 20, 2023

adityamandaleeka commented Mar 16, 2023

adityamandaleeka commented Mar 16, 2023

Kahbazi commented Mar 16, 2023

ghost commented Mar 16, 2023

adityamandaleeka commented Mar 16, 2023

Implement request timeout middleware #46115

Implement request timeout middleware #46115

Conversation

Kahbazi commented Jan 15, 2023

Implement request timeout middleware

Description

ghost commented Jan 15, 2023

JamesNK left a comment • edited Loading

Choose a reason for hiding this comment

JamesNK Jan 16, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Tratcher commented Jan 17, 2023

Tratcher commented Jan 17, 2023 • edited Loading

Tratcher left a comment

Choose a reason for hiding this comment

davidfowl commented Jan 20, 2023

JamesNK commented Jan 20, 2023 • edited Loading

davidfowl commented Jan 20, 2023

Tratcher commented Jan 20, 2023

adityamandaleeka commented Mar 16, 2023

adityamandaleeka commented Mar 16, 2023

Kahbazi commented Mar 16, 2023

ghost commented Mar 16, 2023

adityamandaleeka commented Mar 16, 2023

JamesNK left a comment •

edited

Loading

JamesNK Jan 16, 2023 •

edited

Loading

Tratcher commented Jan 17, 2023 •

edited

Loading

JamesNK commented Jan 20, 2023 •

edited

Loading